Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a different MonoidK and SemigroupK instance for Kleisli #1098

Merged
merged 6 commits into from
Oct 11, 2017
Merged

Add a different MonoidK and SemigroupK instance for Kleisli #1098

merged 6 commits into from
Oct 11, 2017

Conversation

peterneyens
Copy link
Collaborator

@peterneyens peterneyens commented Jun 8, 2016 •
edited
Loading

I was just looking at a StackOverflow question and was thinking this could be solved using SemigroupK.combineK, but while there is a SemigroupK[Option] there is nothing to give us a SemigroupK[λ[α => Int => Option[α]]]. So I have added a SemigroupK and MonoidK for a function A => F[B] if there is a MonoidK[F].

I am not exactly sure how useful this is in practice. This is probably not something used very often (so I am not sure if this warrants inclusion in cats ?). It also seems you will always need to write a type lambda :

import cats.implicits._
val foo: Int => Option[Int] = x => if (x % 2 == 0) Some(x) else None
val bar: Int => Option[Int] = x => if (x % 3 == 0) Some(x) else None
SemigroupK[λ[α => Int => Option[α]]].combineK(foo, bar)

It would of course be nicer if you could write foo <+> bar. I'm not sure if that would be possible using the existing semigroupSyntaxU or using the SI-2712 fix (I tried with a locally published cats version with this PR and Miles' compiler plugin, but couldn't get it working) ?

@codecov-io
Copy link

codecov-io commented Jun 8, 2016 •
edited
Loading

Codecov Report

Merging #1098 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1098      +/-   ##
==========================================
+ Coverage   96.07%   96.07%   +<.01%     
==========================================
  Files         273      273              
  Lines        4538     4539       +1     
  Branches      119      117       -2     
==========================================
+ Hits         4360     4361       +1     
  Misses        178      178
Impacted Files Coverage Δ
core/src/main/scala/cats/data/Kleisli.scala 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cce29c...598e677. Read the comment docs.

@peterneyens
Copy link
Collaborator Author

peterneyens commented Jun 8, 2016 •
edited
Loading

I got the syntax working with a type alias (so it can use the semigroupSyntaxU syntax with an Unapply)

type T[f[_], a, b] = Function[a, f[b]]
val foo: T[Option, Int, Int] = x => if (x % 2 == 0) Some(x) else None
val bar: T[Option, Int, Int] = x => if (x % 3 == 0) Some(x) else None

foo <+> bar

This looks quite a lot like Kleisli 😛 , so I should probably change this PR to add these instances to Kleisli instead of Function1.

@peterneyens peterneyens force-pushed the monoidk-function1-ab branch 3 times, most recently from f6fbd80 to e112955 Compare June 8, 2016 18:24
@peterneyens peterneyens changed the title Add a MonoidK and SemigroupK instance for Function1 (A => F[B]). Add a different MonoidK and SemigroupK instance for Kleisli Jun 8, 2016
@non
Copy link
Contributor

non commented Jun 9, 2016

Looks good to me! 👍

@ceedubs
Copy link
Contributor

ceedubs commented Jun 11, 2016

@peterneyens thanks!

It seems like it could potentially be problematic to have two different SemigroupK instances for Kleisli. Would there be ambiguity in the case of something like Kleilsi[List, Int, Int]?

@peterneyens
Copy link
Collaborator Author

peterneyens commented Jun 11, 2016 •
edited
Loading

@ceedubs You are right, with this PR you will always get the newly added Kleisli[F, A, ?] instances. The existing SemigroupK[F, α, α] will probably be used more and is also consistent with the SemigroupK from Function1.

I'm not sure how often these new instances which combine Kleislis into one with the combined result will be used. Do I leave them in and only make them explicitly available (dropping the implicit from the functions) ?

@edmundnoble
Copy link
Contributor

Any updates on this? I don't see an issue with providing these instances explicitly, provided they're documented.

@peterneyens
Add a MonoidK and SemigroupK instance for Kleisli.
66afdb4 
Combine the result of Kleislis using MonoidK[F] or SemigroupK[F].
@peterneyens
Rebase and make instances explicit
5e2b37f
@peterneyens
Copy link
Collaborator Author

Made this instances explicit (notreally sure about the naming of the instances and the traits) and added doctests for both instances.

Anything else you were thinking of @edmundnoble?

kailuowang
kailuowang previously approved these changes May 28, 2017
View reviewed changes
@rossabaker rossabaker mentioned this pull request Sep 26, 2017
Closed
@kailuowang
Copy link
Contributor

kailuowang commented Oct 4, 2017 •
edited
Loading

@peterneyens do you still have time to work on this PR? If you want, I can continue this one or asking @ChristopherDavenport for the favor

@peterneyens
Copy link
Collaborator Author

Could come back to this in the weekend, but if anybody else wants to go ahead in the mean time, I don't mind.

kailuowang
kailuowang approved these changes Oct 11, 2017
View reviewed changes
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kailuowang
Copy link
Contributor

Thanks so much @peterneyens

@kailuowang kailuowang merged commit 46d0b17 into typelevel:master Oct 11, 2017
@peterneyens peterneyens deleted the monoidk-function1-ab branch October 11, 2017 18:37
@aeons
Copy link
Member

aeons commented Oct 11, 2017

Awesome!

@kailuowang kailuowang added this to the 1.0.0-RC1 milestone Oct 13, 2017
@strong-zero strong-zero mentioned this pull request Jan 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kailuowang kailuowang kailuowang approved these changes

@LukaJCB LukaJCB LukaJCB approved these changes

Assignees
No one assigned
Labels
enhancement Source Breaking
Projects
None yet
Milestone
1.0.0-RC1
Development

Successfully merging this pull request may close these issues.
8 participants
@peterneyens @codecov-io @non @ceedubs @edmundnoble @kailuowang @aeons @LukaJCB